feat(server): inject caller's Nostr pubkey into request extensions#99
Conversation
Add `ClientPubkey` type and inject the caller's Nostr public key (hex) into every inbound request's `extensions` typemap. This allows tool/resource/prompt handlers to identify the caller via `ctx.extensions.get::<ClientPubkey>()`. The feature is always on, unlike the TS adapter's opt-in `_meta` field, and uses rmcp's typed extensions (local-only). The inbound event id remains accessible as `ctx.id`.
Expose the complete verified Nostr event via `InboundEvent` in `extensions` so handlers can access `sig`, `id`, `pubkey`, `tags`, etc. This eliminates the need for handlers to fabricate synthetic events when binding tool calls to the original publishing event (e.g., an MLS key-package coordinator returning the publication event). Also add an `event` field to `IncomingRequest` for the channel seam. FFI types (`FfiIncomingRequest`, the UniFFI `IncomingRequest`) intentionally do not mirror the new field yet because no FFI consumer requires the raw Nostr event, and exposing `nostr_sdk::Event` across the C ABI + UniFFI is non-trivial. Omission is documented in place.
…EP-41) Implement sender-side keepalive for OpenStreamWriter to prevent resource leaks when a client silently disconnects (crash, sleep, network drop) without sending an abort frame. Previously only reader sessions ran keepalive timers, so pure producer streams (e.g. subscription tools) were never probed, leaving writers and downstream producers alive indefinitely. - Add idle_timeout / probe_timeout to OpenStreamWriterOptions to enable keepalive (disabled by default, e.g. for tests). - Arm an idle window when the writer starts streaming; the server keepalive sweep now probes writers via the new `tick` method. - Route inbound pong frames for the stream to the writer to clear probes. - On probe timeout, abort with "Probe timeout", flush a terminal response via the existing on_abort hook, and evict the dead client’s session (mirroring the TS SDK 0.13.8 fix, per CEP-41’s “release local state”). - Reuse the reader idle/probe timeout knobs (one idle/probe pair per stream). - Per CEP-41, only inbound frames reset the idle window; relay writes do not. Closes the silent-disconnect leak and aligns with CEP-41 requirements.
There was a problem hiding this comment.
Went through this against ts-sdk 0.13.8 piece by piece. Solid change, docs and tests are thorough. One thing I'd sort before merge, then a few smaller notes.
The parity story holds up: TS's injectClientPubkey is opt-in (default off, nostr-server-transport.ts:229) and writes to the on-wire _meta (utils.ts:47), so doing it always-on through local rmcp extensions instead is a reasonable call.
The plaintext path doesn't verify the event signature, but ts-sdk does. This is half of ts-sdk's identity-forgery fix (ContextVM/sdk#64, PR ContextVM/sdk#69). That fix verified both inbound paths: the decrypted inner event, and the unencrypted one (a separate commit, fix: verify unencrypted event signatures). We port the first at server/mod.rs:1698 (inner.verify()), but the plaintext else arm (server/mod.rs:1741-1759) trusts event.pubkey without ever calling verify(). ts-sdk verifies it in handleUnencryptedEvent (event-pipeline.ts:162), and the reasoning is on the encrypted path just above (event-pipeline.ts:113-116): an attacker can put any pubkey in the plaintext and bypass allowlists. We gate the auth allowlist on exactly that pubkey (server/mod.rs:1789), so it lands here too.
To be clear it isn't exploitable on the default transport: nostr-sdk's RelayPool already verifies inbound signatures (nostr-relay-pool-0.44.1, relay/inner.rs:1233), which is the whole reason ts-sdk has to do it in-app, since nostr-tools doesn't. It only bites with a custom RelayPoolTrait that skips verification, like our own MockRelayPool. RelayPoolTrait is public, so as written, caller identity quietly depends on which pool someone plugs in. The gap predates this PR, but this is the change that makes it matter, since it promotes event.pubkey into first-class handler identity. I'd just mirror the gift-wrap arm:
// verify() also checks event.id matches its content, which we rely on for
// correlation and the request id below. Redundant against the default pool,
// but it keeps identity independent of the pool impl.
if let Err(e) = event.verify() {
tracing::warn!(target: LOG_TARGET, "Plaintext event signature verification failed: {e}");
continue;
}Not a hard blocker given the default pool, but it's cheap insurance, and I'd rather caller identity not hinge on an undocumented assumption about the transport.
A few smaller things:
Scope / description. Three separate changes are riding together: ClientPubkey (the parity piece), InboundEvent, and the CEP-41 open-stream fix. The description only really covers the first, so it might be worth mentioning all three, since the CEP-41 fix isn't described at all and a reviewer could miss it.
InboundEvent. No issue with it going beyond the reference. TS only surfaces the event lazily via getNostrRequestEvent (nostr-server-transport.ts:627), and injecting it directly is nicer. One real gap though: it's silently None for oversized (CEP-22) requests. The reassembly path sends a real sender_pubkey with event: None (server/mod.rs:2211-2214), so a handler can't tell "no event" from "the request was too big to fit in one." That's exactly the MLS key-package case the description pitches, since a key package is the kind of payload that goes oversized. At least worth making the docs honest about it.
ClientPubkey on synthetic requests. It's injected unconditionally, so the internal announcement/init requests get a ClientPubkey that's really the ANNOUNCEMENT_REQUEST_ID sentinel (InboundEvent is correctly None there). ts-sdk only ever injects a real event.pubkey. Might be worth skipping it for those, but gate on the sentinel value, not on event.is_none(), since the oversized path above carries a real pubkey with no event.
Couple of things I checked that are right as-is: evicting the whole session on a writer probe timeout matches ts-sdk's handleProbeTimeout (open-stream-factory.ts:307), and ClientPubkey(String) as raw hex is correct, since a typed key would drift from _meta.clientPubkey: string.
Really nice work overall. The encrypted-path verification, per-request extension isolation, the whoami/whoami_event coverage across all three encryption modes, the keepalive tests plus the silent-client regression, and documenting the FFI omission instead of leaving it implicit all stood out.
…nit drives The transport's own announcement and initialization drives use the `ANNOUNCEMENT_REQUEST_ID` sentinel as their pubkey. Previously, this sentinel was unconditionally injected into request extensions, handing handlers a bogus caller identity. Now guard the injection so that only real client pubkeys (including those from oversized CEP-22 requests) are exposed via `ClientPubkey`.
harsh04044
left a comment
There was a problem hiding this comment.
LGTM, all items addressed! Just worth updating the PR description to cover InboundEvent and the CEP-41 fix, since right now it only mentions the pubkey injection.
Add
ClientPubkeytype and inject the caller's Nostr public key (hex) into every inbound request'sextensionstypemap. This allows tool/resource/prompt handlers to identify the caller viactx.extensions.get::<ClientPubkey>(). The feature is always on, unlike the TS adapter's opt-in_metafield, and uses rmcp's typed extensions (local-only). The inbound event id remains accessible asctx.id.